Add Mount(..., middleware=[...]) #1649
Conversation
adriangb
requested review from
Kludex,
tomchristie,
florimondmanca and
abersheeran
|
This would be really useful. Is there a plan to merge and release this still at some point? |
|
I'll review this before the next minor release. |
|
The implementation is simple and works well. However, there is a different behavior in both if an exception raised in handler. This exception will be handled by I wonder if this behavior changing is expected and should be documented or we need build the middleware stack per |
Co-authored-by: Marcelo Trylesinski <[email protected]>
|
I think we can take this opportunity to add a more real case usage instead of |
I was going to say yes but: #1837. So |
|
There was a problem hiding this comment.
Thanks @adriangb 🎉
I think it will be great if we can add a test that fails with the behavior you mentioned on the documentation, and I know you like those kinds of tests... 😎 👍 What do you think?
A bit of reference about this PR, because my memory is short, and I like to make things clear:
There were two previous attempts to solve the issue, which the first one was actually approved (#1286), but I pushed back. On the second one (#1464), @florimondmanca suggested adding a middleware parameter only on the Mount class, and other members agreed, so it was rejected as well. And now, I think we found the solution that makes more sense. Short solution, great improvement. 👍
tests/test_routing.py
Outdated
| "route", | ||
| [ | ||
| mounted_routes_with_middleware, | ||
| mounted_routes_with_middleware, |
There was a problem hiding this comment.
Duplicated as well, so I think we can get rid of the @parametrize.
| route: BaseRoute, | ||
| ) -> None: | ||
| test_client = test_client_factory(Router([route])) | ||
| response = test_client.get("/http") |
There was a problem hiding this comment.
Should we also assert that calling a route not under the Mount does not apply the middleware? E.g. testing a test_client.get("/").
docs/middleware.md
Outdated
| Note that middleware used in this way is *not* wrapped in exception handling middleware like the middleware applied to the `Starlette` application is. | ||
| This is often not a problem because it only applies to middleware that inspect or modify the `Response`, and even then you probably don't want to apply this logic to error responses. | ||
| If you do want to apply the middelware logic to error responses only on some routes you have a couple of options: | ||
|
|
||
| * Add an `ExceptionMiddleware` onto the `Mount` | ||
| * Add a `try/except` block to your middleware and return an error response from there | ||
| * Split up marking and processing into two middlewares, one that gets put on `Mount` which simply marks the response as needing processing (for example by setting `scope["log-response"] = True`) and another applied to the `Starlette` application that does the heavy lifting. |
There was a problem hiding this comment.
I'm not sure I understand why requests to sub-mounted error routes bypass middleware associated with that mount? Or maybe that's not the problem here? I'm confused. Isn't ErrorMiddleware supposed to wrap the entire Starlette.router, which includes sub-apps?
docs/middleware.md
Outdated
| Note that middleware used in this way is *not* wrapped in exception handling middleware like the middleware applied to the `Starlette` application is. | ||
| This is often not a problem because it only applies to middleware that inspect or modify the `Response`, and even then you probably don't want to apply this logic to error responses. | ||
| If you do want to apply the middelware logic to error responses only on some routes you have a couple of options: | ||
|
|
||
| * Add an `ExceptionMiddleware` onto the `Mount` | ||
| * Add a `try/except` block to your middleware and return an error response from there | ||
| * Split up marking and processing into two middlewares, one that gets put on `Mount` which simply marks the response as needing processing (for example by setting `scope["log-response"] = True`) and another applied to the `Starlette` application that does the heavy lifting. |
There was a problem hiding this comment.
Lemme try to draw this out to make sure I understand...
Looking at the Starlette code, we have this layering:
routes = [
Route("/", ...),
Mount("/sub", sub, middleware=[Middleware(MountMiddlewareA)]),
]
app = Starlette(
routes=routes,
middleware=[Middleware(MiddlewareB), Middleware(MiddlewareC)],
)ServerErrorMiddleware(
MiddlewareC(
MiddlewareB(
ExceptionMiddleware(
Router(
routes=[
Route("/", ...),
Mount("/sub", MiddlewareA(sub))
]
)
)
)
)
)So, if an exception occurs in /, it gets handled by ExceptionMiddleware and Middleware{B,C} get a well-defined response [perhaps an error response] to process.
But if an exception occurs in /sub*, it gets sent to MiddlewareA, which has to handle it if it hopes to do some response processing (otherwise that processing isn't run).
Is that right?
|
I'm going to go ahead and merge it so we can proceed with the next release, if anyone has any last minute concerns we can address them in a separate PR before the release. |
|
A thought about this PR, Request objects and error handling. I wonder if we maybe should’ve had ExceptionMiddleware wrap each Route instead of being a middleware at the application level? That would have both resolved the concerns in #1649 (comment) and also #1692 (comment) because we’d be able to (1) make sure there is nothing in between ExceptionMiddleware and the route and (2) use the same Request object for error handlers and the endpoint. I wonder if this could be made backwards compatible… |
Co-authored-by: Marcelo Trylesinski <[email protected]> Co-authored-by: Florimond Manca <[email protected]> Co-authored-by: Aber <[email protected]>
By @Kludex: